[3/n] Enable validation composability and add composable single singer validation and tests #96

fangting-alchemy · 2024-07-12T19:17:39Z

Update interfaces to support validation composability.
Add an ECDSA validation plugin and tests

src/plugins/owner/SingleOwnerPlugin.sol

adamegyed · 2024-07-16T15:59:14Z

src/plugins/validation/EcdsaValidation.sol

+///
+/// - This validation supports composition that other validation can relay on entities in this validation
+/// to validate partially or fully.
+contract EcdsaValidation is IEcdsaValidation, BasePlugin {


Could you clarify how this plugin is different from SingleOwnerPlugin?

The plan is to delete SingleOwnerPlugin after this as it essentially supports a subset feature of EcdsaValidation.
The features of EcdsaValidation is in the docs above.

I think SingleOwnerPlugin has one other feature that EcdsaValidationPlugin doesn't support, which is contract owners via ERC-1271. That could be worth keeping it as the primary plugin, and updating it to be composable maybe? Wdyt?

EcdsaValidation supports contract owners via ERC-1271 too.

Ah, sorry I missed that. In that case, isn't Single owner a more applicable name than ECDSA? Since it's not always ECDSA?

hmmm, what do you mean? What could be an owner for SingleOwner that cannot be a signer for EcdsaValidation?

I meant "SingleOwnerPlugin" is possibly a more fitting name. Via 1271, the signature validation is no longer just ECDSA (using ecrecover), because the owner contract can implement any type of check.

Gotcha. Good point. Maybe SingleSigner is a bit more fitting.

But then why are we recreating it in a new plugin contract, rather than updating SingleOwnerPlugin, if it's doing the same thing? I think we're one line away from equivalence, with the update to userOp.sender

They are very similar in the end (each func will need some update). Creating the validation and add tests was a lot easier for the development process than updating at the time.
Also it is a minor repo structure improvement with validation v.s. owner folder.
With PR [4/n], it does not matter how we get to here, right?

adamegyed · 2024-07-16T16:01:56Z

test/utils/TestConstants.sol

@@ -2,3 +2,4 @@
 pragma solidity ^0.8.25;

 uint32 constant TEST_DEFAULT_OWNER_FUNCTION_ID = 0;
+uint32 constant TEST_DEFAULT_VALIDATION_ID = 1;


Why not reuse? Left a comment up on the stack about renaming the above from TEST_DEFAULT_OWNER_FUNCTION_ID to TEST_DEFAULT_OWNER_ENTITY_ID.

adamegyed · 2024-07-16T16:51:28Z

src/interfaces/IValidation.sol

@@ -27,6 +28,7 @@ interface IValidation is IPlugin {
    /// @param data The calldata sent.
    /// @param authorization Additional data for the validation function to use.
    function validateRuntime(


Thanks for updating these other two validation functions - with this, we should have full composability across all of the different types of validation functions.

… in ecdsaValidation

howydev · 2024-07-16T22:59:22Z

src/account/UpgradeableModularAccount.sol

-        if (IValidation(plugin).validateSignature(entityId, msg.sender, hash, signature[24:]) == _1271_MAGIC_VALUE)
-        {
+        if (
+            IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:])


not sure if this should be addressed in this PR, but if the return values are _1271_MAGIC_VALUE or _1271_INVALID, would prefer to just remove the if/else part and do return IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:])

I think it's preferable to do the equivalence check before returning, because then we can absorb any bad values that are valid bytes4 types but aren't _1271_MAGIC_VALUE or _1271_INVALID.

mmm from the spec, returning anything thats not _1271_MAGIC_VALUE should be considered invalid. Don't feel super strongly about this, just a LOC reduction maxi

howydev · 2024-07-16T23:43:24Z

src/plugins/validation/SingleSignerValidation.sol

+    /// @inheritdoc IPlugin
+    function onUninstall(bytes calldata data) external override {
+        // ToDo: what does it mean in the world of composable validation world to uninstall one type of validation
+        // We can either get rid of all SingleSigner signers. What about the nested ones?


I think we have to support the flow of getting rid of 1. Use case would be something like revoking 1 dapp session key by uninstalling the validation for that entity. Open to having a global "uninstall all" type of feature though

howydev

Agree with Adam on reusing TEST_DEFAULT_VALIDATION_ID if possible, otherwise LGTM

fangting-alchemy requested a review from a team July 12, 2024 19:17

fangting-alchemy force-pushed the fangting/ecdsa-validation branch from cc54fc2 to e0b605f Compare July 12, 2024 20:33

adamegyed reviewed Jul 16, 2024

View reviewed changes

fangting-alchemy requested a review from adamegyed July 16, 2024 16:31

adamegyed reviewed Jul 16, 2024

View reviewed changes

fangting-alchemy added 5 commits July 16, 2024 10:29

add ecdsa validation

d22bec9

update ecdsa validation and add more test

03f9cf3

add account as passed in param in validate runtime

7488ffd

add account as param in validateSignature method and add 1271 support…

6fb6dee

… in ecdsaValidation

rename ecdsa to SingleSigner

7685790

fangting-alchemy force-pushed the fangting/ecdsa-validation branch from 3c43b10 to 7685790 Compare July 16, 2024 17:29

fangting-alchemy changed the title ~~[3/n] Add composable ecdsa validation and tests~~ [3/n] Add composable single singer validation and tests Jul 16, 2024

howydev reviewed Jul 16, 2024

View reviewed changes

howydev approved these changes Jul 16, 2024

View reviewed changes

fangting-alchemy and others added 2 commits July 16, 2024 17:12

rename validationId in SingleSignerValidation to entityId

d66b6b8

[4/n] validation series - delete SingleOwnerPlugin (#97)

dc9ed79

fangting-alchemy merged commit 52391bb into validation Jul 17, 2024
3 checks passed

fangting-alchemy deleted the fangting/ecdsa-validation branch July 17, 2024 00:26

fangting-alchemy added a commit that referenced this pull request Jul 17, 2024

[3/n] Add composable single singer validation and tests (#96)

0ddd484

fangting-alchemy changed the title ~~[3/n] Add composable single singer validation and tests~~ [3/n] Enable validation compatibility and add composable single singer validation and tests Jul 22, 2024

fangting-alchemy changed the title ~~[3/n] Enable validation compatibility and add composable single singer validation and tests~~ [3/n] Enable validation composability and add composable single singer validation and tests Jul 22, 2024

fangting-alchemy mentioned this pull request Jul 22, 2024

[Improvement] Validation composability erc6900/resources#36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3/n] Enable validation composability and add composable single singer validation and tests #96

[3/n] Enable validation composability and add composable single singer validation and tests #96

fangting-alchemy commented Jul 12, 2024 •

edited

Loading

adamegyed Jul 16, 2024

fangting-alchemy Jul 16, 2024

adamegyed Jul 16, 2024

fangting-alchemy Jul 16, 2024

adamegyed Jul 16, 2024

fangting-alchemy Jul 16, 2024 •

edited

Loading

adamegyed Jul 16, 2024

fangting-alchemy Jul 16, 2024

adamegyed Jul 16, 2024

fangting-alchemy Jul 16, 2024

adamegyed Jul 16, 2024

adamegyed Jul 16, 2024

howydev Jul 16, 2024

adamegyed Jul 16, 2024

howydev Jul 16, 2024

howydev Jul 16, 2024

howydev left a comment

[3/n] Enable validation composability and add composable single singer validation and tests #96

[3/n] Enable validation composability and add composable single singer validation and tests #96

Conversation

fangting-alchemy commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangting-alchemy Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howydev left a comment

Choose a reason for hiding this comment

fangting-alchemy commented Jul 12, 2024 •

edited

Loading

fangting-alchemy Jul 16, 2024 •

edited

Loading